feat(provider): add official xAI Grok CLI support - #596
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
=======================================
Coverage ? 91.37%
=======================================
Files ? 183
Lines ? 25148
Branches ? 0
=======================================
Hits ? 22979
Misses ? 2169
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fanhongy
left a comment
fanhongy
left a comment
There was a problem hiding this comment.
Summary
The provider registration, command construction, restrictions, MCP configuration, and focused tests are in place. I found two reproducible P2 defects in multi-turn status handling and restart cleanup.
Findings
P2: Long turns with the same elapsed-time marker can never complete
In src/cli_agent_orchestrator/providers/grok_cli.py:438, when the current query has fallen outside the 8,192-character status tail, turn_start is set to the completion marker itself. The fingerprint at lines 440-447 then contains only text such as Worked for 2.0s. If a later long turn reports the same elapsed duration, its otherwise valid completion is treated as the previous turn and get_status() returns PROCESSING indefinitely.
I reproduced this with two different queries and two different 9,000-character response streams, each ending in a structurally valid raw Worked for 2.0s marker and ready footer: the first returned completed, while the second returned processing. Because send_input() clears the rolling buffer and the status monitor consumes its processing-revert arm, handoff/inbox workflows can then wait until timeout despite Grok being visibly ready.
Preserve a current-turn discriminator before the query is evicted from the tail, or otherwise tie completion evidence to the dispatch/buffer generation instead of reducing the fingerprint to the elapsed-time marker. Add a regression test with two long, distinct turns sharing the same duration.
P2: Deleting a restored terminal leaves its private Grok home behind
src/cli_agent_orchestrator/providers/grok_cli.py:509 makes cleanup a no-op whenever _grok_home is None. That field is only assigned by _prepare_grok_home() during initialization. After cao-server restarts, ProviderManager reconstructs the provider from database metadata without running initialization, so the restored provider has _grok_home=None; if no provider has yet been reconstructed, cleanup_provider() is also a no-op. Deleting that terminal therefore leaves <CAO_HOME>/grok/terminals/<id>-<hash> permanently on disk.
I reproduced the lifecycle by preparing a home, discarding the provider object to simulate restart, constructing the restored provider, and calling cleanup(); home.exists() remained true. The retained directory contains config.toml and may contain MCP headers/environment values plus the live auth.json symlink, contradicting the documented cleanup behavior.
Make Grok cleanup reconstruct the deterministic path from terminal_id and ensure terminal deletion invokes that cleanup even when the in-memory provider map was lost. Add a restart/delete regression test.
Validation
- Read all acquisition artifacts and the complete 40-file diff at head
a7c09a4ae467a09bdbe3a64875beb1ec24d32bf0. - Ran focused provider, manager, tool-mapping, and terminal-service tests:
130 passed. - Ran
git diff --check: passed. - Ran two read-only Python reproductions for the findings above.
- Acquisition validation reported 340 focused Python tests, 175 web tests, and the production web build passing.
- Live Grok E2E was not run because
grokis not installed in the review environment.
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #596 — feat(provider): add official xAI Grok CLI support
Summary
Adds the official xAI Grok Build CLI as a first-class grok_cli provider: registration across CLI/API/manager/web, an isolated per-terminal GROK_HOME with atomic 0600 MCP config and symlinked (never copied) auth, hard tool restrictions via native --deny rules that survive --always-approve, careful multi-turn TUI status detection with raw/rendered fixtures, and thorough docs. The implementation quality is well above the bar for a new provider — it follows the established claude_code/copilot_cli hard-enforcement pattern and the codex/antigravity auto-approve pattern, ships a 619-line unit suite plus a full e2e matrix, and the security-relevant file handling (atomic write, mode repair, cleanup on init failure) is unusually careful. Recommend approve; two questions below are worth an author response but neither is a demonstrated defect.
Important (should fix or answer)
- [security] src/cli_agent_orchestrator/utils/tool_mapping.py (grok_cli.execute_bash) —
execute_bashmaps to only["Bash"], while the claude_code mapping neededBash, BashOutput, KillShell, Task, Agent, Monitorafter live escapes through subagent/background-shell tools. Grok's subagent path is closed by unconditional--no-subagents(and CAO owns theconfig.tomlit could be re-enabled from), but please confirm Grok 1.0.0's full native tool inventory contains no other execution-capable surface (background-shell output/kill companions, monitor-style tools, or anything that can shell out) that would stay approvable under--always-approveon a restricted profile. The passing restricted-supervisor e2e covers theBashand write paths; it can't prove the inventory is exhaustively covered. Weighted up because this is the hard-enforcement security boundary. - [correctness] src/cli_agent_orchestrator/providers/grok_cli.py:get_status (stale-completion fingerprint guard) — the guard hashes the last-query→
Worked forslice and reports PROCESSING while the fingerprint is unchanged after a new dispatch. If two consecutive turns produce a byte-identical slice (same prompt, same response, same thought/work durations — plausible for short repeated orchestration prompts like a heartbeat "ok"), the second completion is indistinguishable from the stale frame and the turn wedges in PROCESSING until any differing frame arrives. The tests show this trade-off is deliberate and the window is narrow, but consider mixing a monotonic cue (e.g., completion-match count in the full buffer) into the guard, or documenting the failure mode and its recovery. Weighted up because it sits in provider status detection.
Nits (optional)
- [security] grok_cli.py:_build_grok_command — the full profile system prompt plus runtime skill catalog is passed as a single
--rulesargv element, visible to other local users viapsand bounded by ARG_MAX for very large skill catalogs. Antigravity does the same via-i, so there's precedent, but codex's file-based developer-instructions approach is the more robust pattern if Grok supports rules-from-file. - [correctness] grok_cli.py:_prepare_grok_home (auth symlink) — if a future Grok build refreshes tokens by atomically replacing
auth.json, the replace converts the symlink into a regular file inside the disposable home: the refreshed token is deleted at cleanup and the real~/.grok/auth.jsonkeeps the stale one. Works with current observed behavior (in-place write-through); worth a one-line comment so the assumption is explicit. - [correctness] grok_cli.py:initialize — an unauthenticated launch parks on the login picker (correctly classified WAITING_USER_ANSWER) and then surfaces as a generic "initialization timed out" error. Including a hint ("is Grok authenticated? run
grok login") in the timeout message would save operators a diagnosis step; docs do cover the prerequisite. - [consistency] test/e2e/test_allowed_tools.py — the added
Path(BASH_MARKER_FILE).unlink(missing_ok=True)cleanup in the shared helpers changes behavior for all providers' tests, not just Grok. It's a genuine leak fix and harmless, just slightly out of the PR's stated scope.
Tests
Excellent coverage for a provider PR. The 619-line unit suite exercises status detection across rendered and raw pipe-pane fixtures (idle, processing, completed, permission picker, login, telemetry banner, error, second turn, ANSI/CUP redraws), stale-marker ordering, the multi-turn fingerprint guard, message extraction with chrome/thought/timestamp stripping, command construction (flag set, model precedence, deny mapping, web kill-switch, empty allowlist), private-home isolation, atomic 0600 config writes with mode repair, auth symlinking (including custom GROK_HOME), idempotent/retryable cleanup, and async init success/timeout/cleanup paths. Registration is covered in test_constants.py, test_provider_manager_unit.py, test_terminal_service.py, API and launch tests, and the tool-mapping table in test_tool_mapping.py. The e2e matrix adds Grok classes to allowed-tools (restricted + unrestricted + a new read-only reviewer write-denial probe), assign, handoff, send_message, skills, and supervisor orchestration, all gated on a require_grok fixture that reuses auth by symlink without copying credentials. Fixtures are sanitized (login code is XXXX-XXXX; no PII observed).
Verification
Ran the focused suites in the PR worktree with the repo venv (Python 3.12):
- ✓ VERIFIED
test/providers/test_grok_cli_unit.py,test_provider_manager_unit.py,test/utils/test_tool_mapping.py,test/test_constants.py,test/services/test_terminal_service.py— 194 passed. - ✓ VERIFIED
test/api/test_api_endpoints.py,test/cli/commands/test_launch.py— 146 passed. - ✓ VERIFIED the PR body's "Focused Python suite: 340 passed" claim (194 + 146 = 340, matching exactly).
- ⁇ NOT VERIFIED: the Grok tmux e2e matrix (14/14 claimed) and live TUI marker behavior — the
grokbinary is not installed on this host; e2e tests skip viarequire_grok. Manual: install Grok Build 1.0.0, authenticate, thenuv run pytest -m e2e test/e2e/ -k grok -v. - ⁇ NOT VERIFIED: frontend tests (Node unavailable here, same as the author's environment); the web change is a one-element fallback-list addition with a matching test update — CI should exercise it.
Verdict
Approve with nits — a carefully engineered provider addition with strong isolation, hard enforcement, and verification; the two Important items are questions/hardening in sensitive paths rather than demonstrated defects.
|
|
||
| ## E2E Testing | ||
|
|
||
| The `data_analyst` and `report_generator` profiles from this directory are used in the E2E test suite to validate assign and handoff flows across all providers (codex, claude_code, kiro_cli, kimi_cli). |
There was a problem hiding this comment.
@thuanlm215 why are we crossing out the existing providers ?
There was a problem hiding this comment.
No intent to cross out any provider. That wording was corrected in the merge update at 4b19e7c; the README now says the examples validate the supported providers, including grok_cli. Thanks for catching it.
There was a problem hiding this comment.
@thuanlm215 thanks for the great work! I tested this provider against the official Grok Build 1.0.0 binary, not only the fixtures. I am requesting changes for five independently reproduced P2 issues.
The previously reported long-turn status wedge and restart cleanup leak are both real. I also reproduced three additional gaps: Grok workflows can still start native subagents in CAO-controlled mode, project-local configuration stops startup at an unhandled trust screen, and SSE MCP profiles are rewritten as ordinary HTTP profiles.
I checked the separate monitor concern as well: Grok reports that --deny Bash blocks the monitor tool, so I am not treating that as a defect. The focused Python suite (340 tests), web suite (175 tests), and production web build pass at this head. I could not run the account-backed assign/handoff E2E because no authorized Grok account was available.
| binary, | ||
| "--no-alt-screen", | ||
| "--always-approve", | ||
| "--no-subagents", |
There was a problem hiding this comment.
[P2] Make CAO-controlled mode cover workflow subagents
--no-subagents hides Grok's direct spawn_subagent tool, but it does not stop the workflow tool or /goal from starting Grok-native workers. With Grok 1.0.0 and these exact TUI flags, I had the model call an inline workflow containing one agent() step; the workflow completed and produced a separate Grok Build subagent model request. /goal also displayed Ran 1 subagent.
Those workers are invisible to CAO's terminal/group accounting and do not use CAO's role profiles or assign callbacks, so a supervisor can delegate work outside CAO even though this flag and the docs say that path is closed. Please make CAO-controlled mode actually disable workflow-backed workers. Keep native loops available through an explicit mode so /goal and provider-native workflows remain usable when the user chooses them, rather than silently mixing the two control models.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: --no-subagents only covered the direct tool, not workflow-backed workers or /goal. CAO-controlled launch now also sets GROK_SUBAGENTS=0, GROK_WORKFLOWS=0, and GROK_GOAL=0; native behavior requires the typed per-profile grokNativeWorkflows: true opt-in, which sets them to 1 and omits --no-subagents. Regression tests cover default and opt-in command construction/profile validation. I also ran focused Grok 1.0.0 probes for the default and opt-in paths; the default did not start a native worker, while the explicit opt-in did. This is documented with the version caveat because these controls are not all exposed by grok --help.
| await asyncio.to_thread( | ||
| get_backend().send_keys, self.session_name, self.window_name, command | ||
| ) | ||
| if not await wait_until_status( |
There was a problem hiding this comment.
[P2] Handle Grok's directory-trust screen before waiting for ready
Every terminal gets a fresh private GROK_HOME, so it has no saved folder-trust decision. In a working directory containing only .mcp.json, official Grok 1.0.0 stops at Do you trust the contents of this directory? before it renders the composer. That screen matches neither WAITING_USER_PATTERN nor a ready state, so this wait runs until provider_init_timeout, then initialization deletes the terminal. I reproduced this in the real TUI.
Please handle this startup screen using an explicit safe policy and add a regression fixture. Simply pressing Yes unconditionally needs care because the process also runs with --always-approve, and trusting the folder enables repository MCP/hooks.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: initialization waited only for ready/completed status, so the project directory-trust surface was treated as a generic timeout. The ready wait now inspects the status buffer first and raises an actionable ProviderError on the exact trust screen; initialization cleanup then removes the private home. CAO deliberately never answers Yes because that trusts repository-local MCP, LSP, and hooks under the terminal user. Added trust-screen fixture/regression coverage and the operator guidance in docs/grok-cli.md. Focused live Grok 1.0.0 trust-screen probing confirmed detection follows the safe fail-closed path.
| table = f"mcp_servers.{_toml_string(name)}" | ||
| lines.extend(["", f"[{table}]"]) | ||
| if config.get("url"): | ||
| lines.append(f"url = {_toml_string(config['url'])}") |
There was a problem hiding this comment.
[P2] Keep the SSE transport in generated MCP config
This URL branch drops the profile's type field. Grok 1.0.0 requires type = "sse" for an SSE server; its own grok mcp add --transport sse ... command writes that field. Without it, Grok treats the same URL as ordinary HTTP. I rendered an SSE profile with this code and confirmed with grok mcp list --json that the generated entry was no longer SSE. Preserve the transport field and add an SSE config test.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: the URL MCP rendering branch emitted only url, silently relying on Grok HTTP defaulting and thereby changing an SSE profile. Generated TOML now preserves explicit type = "http" or type = "sse" and rejects unsupported URL transports rather than emitting a changed configuration. Regression coverage asserts both HTTP and SSE output.
| completion_match = completion_matches[-1] | ||
| query_matches = list(QUERY_PATTERN.finditer(tail[: completion_match.start()])) | ||
| turn_start = ( | ||
| query_matches[-1].start() if query_matches else completion_match.start() |
There was a problem hiding this comment.
[P2] Do not identify a long turn only by its elapsed-time text
Once a response is long enough to push its query outside this 8,192-character tail, turn_start becomes the completion marker itself. The fingerprint then contains only text such as Worked for 2.0s. A later, different long turn with the same reported duration is mistaken for the old completion and remains PROCESSING forever. I reproduced two different 9,000-character turns: the first returned COMPLETED, the second returned PROCESSING. Tie completion to the current dispatch/buffer generation instead, and add that two-turn regression case.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: the stale guard used a fingerprint from the 8 KiB display tail; once the query was evicted, it degraded to the shared Worked for 2.0s marker. The provider now tracks a monotonic normalized-stream position across rolling-buffer overlap and combines it with full-transcript completion identity/current-turn activity. A retained stale completion remains PROCESSING, while a later identical marker at an advanced stream position completes. Regressions cover two >9 KiB distinct turns with equal durations, raw and rendered output, byte-identical consecutive turns, and 1 KiB rolling-buffer eviction. Focused regression suite passed locally (223 tests); full static validation also passed (318 passed, 10 skipped).
| def cleanup(self) -> None: | ||
| self._initialized = False | ||
| home = self._grok_home | ||
| if home is None: |
There was a problem hiding this comment.
[P2] Remove the private Grok home after a server restart
_grok_home exists only on the provider object that ran initialization. After cao-server restarts, deletion either finds no provider in the manager or reconstructs one with _grok_home = None, so this early return leaves the deterministic terminal home on disk. I reproduced prepare -> discard provider -> reconstruct -> cleanup, and the directory remained. The retained directory can contain generated MCP environment/header values and the live auth.json symlink. Derive the path from terminal_id during cleanup and make deletion call that cleanup even when the in-memory provider map was lost.
There was a problem hiding this comment.
Fixed across 3bcab70 and 6d9e392. Root cause: cleanup depended on the initialized provider object, which is absent after a server restart. Grok homes are now deterministically reconstructed from terminal id, and ProviderManager creates a cleanup-only Grok adapter from persisted terminal metadata when its in-memory map is empty. Cleanup validates the exact managed path (including symlinked-ancestor defenses), waits for exact-home residual Grok/MCP processes to stop, then removes the home. Terminal/flow teardown now kills the owning tmux process before provider cleanup to avoid updater recreation races. Regression coverage includes restart/delete, idempotency, retryable process cleanup, and symlink-escape handling; focused live cleanup probes and the focused unit suite passed.
|
@haofeif All five requested P2 fixes are now pushed in 3bcab70 and 6d9e392, with inline root-cause and test details on each thread. Validation now includes:
The full E2E run covered allowed-tools, assign, handoff, inbox messaging, skills, and supervisor orchestration. Could you please re-review when convenient? |
There was a problem hiding this comment.
Thanks for the thorough fixes @thuanlm215 . The native-workflow opt-in, trust-screen handling, SSE transport, and the original distinct long-turn case now behave as intended. The Linux restart-cleanup happy path also works.
I found two remaining P2 correctness gaps in the new state and cleanup logic: a fast repeated turn can still stay PROCESSING forever after the real buffer clear, and cleanup permanently leaks every private Grok home when Linux /proc is unavailable (including supported macOS). Both are independently reproduced in the inline comments.
I checked this exact head (6d9e392) against official Grok Build 1.0.0 and ran the focused provider/lifecycle/profile tests (124 passed). Account-backed Grok E2E was not available in this review environment.
|
Think we are getting very close. Very keen to get this PR merged |
|
@haofeif Thanks again for the careful review. I’ve addressed the two remaining P2s and pushed the fixes in |
fanhongy
left a comment
fanhongy
left a comment
There was a problem hiding this comment.
Summary
Reviewed head 78960b2f5b9da9206b8185bba5985bff2c684473 against the PR intent and base. The provider registration, permission mapping, isolated home setup, and retryable cleanup are broadly covered, but I found two P2 behavioral defects: raw status parsing can complete a turn on ordinary prose, and deferred cleanup is still reported as a successful delete by user-facing clients.
Findings
P2: Keep raw completion ordinals aligned with the status tail
src/cli_agent_orchestrator/providers/grok_cli.py:679
Raw structural completion ordinals are counted over the full output, while clean_counts restarts at zero inside the last 8 KiB tail. Once an earlier raw Worked for 2.0s marker has scrolled outside that tail, second-turn prose containing the same phrase is ordinal zero in the tail and incorrectly matches the old marker's structural ordinal. With a retained ready footer and a preceding processing marker, get_status() then returns COMPLETED even though the current turn has emitted no structural completion marker. This can make orchestration consume or act on an incomplete response. Preserve the full-buffer ordinal offset when scanning the tail (or associate raw structural markers with absolute normalized positions), and add a regression with an old marker outside _STATUS_TAIL_CHARS plus same-duration prose in the active turn.
P2: Propagate deferred cleanup as a failed delete
src/cli_agent_orchestrator/services/terminal_service.py:1717
The new cleanup contract returns False after killing the backend window but retaining the terminal row and Grok home for a required retry. DELETE /terminals/{id} exposes that as HTTP 200 with {"success": false}, but both dashboard delete handlers ignore the payload and show success. The session path similarly records an error and omits the session from deleted (src/cli_agent_orchestrator/services/session_service.py:175), while the API still returns "success": true; the web UI and cao shutdown therefore also report deletion complete. A protected/orphaned process triggers this path, leaving the retry row and private home behind without telling the user to retry. Make deferred cleanup a non-success API result, or require every client to inspect success/errors, preserve the retry identifier, and add terminal/session client tests for the deferred case.
Validation
366 passed: focused provider, manager, lifecycle, status monitor, MCP cleanup, and tool-mapping tests.2 passed: the two flow-service tests noted as failures in the acquisition context, rerun directly under the repository's normal test contract.175 passed: frontend tests (with expected jsdom canvas/error-boundary diagnostics).- Grok provider mypy and Markdown link validation passed.
- A direct parser reproduction returned
completedfor a second turn containing no current structural completion marker. - Live Grok E2E was not run because
grokis not installed in the review environment.
An evicted structural Worked-for marker was still ordinal 0 in the full buffer, while the 8 KiB tail restarted that count. Same-duration prose in the active turn then matched the old marker and returned COMPLETED.
DELETE /terminals and DELETE /sessions now return HTTP 409 when cleanup must be retried, and the dashboard, shutdown CLI, and MCP client inspect that result instead of reporting a successful delete.
|
@fanhongy thanks for catching these — both made sense. I pushed two follow-ups on this:
Would you mind taking another look when you have a chance? |
|
Sure. Thanks for the PR, LGTM, I have approved, pending on @haofeif |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
LGTM. Thank you @thuanlm215 for your great contribution!
|
@thuanlm215 can you please help to fix the CI errors ? |
The deferred-cleanup 409 check used `session_name not in deleted`. A bool or other non-sequence mock then raised TypeError and became HTTP 500.
|
Fixed the CI failure in |
Brings in #608 (bearer token on the terminal WebSocket), #622 (replace a vulnerable image-size dependency) and #596 (xAI Grok CLI provider). No conflicts, despite #596 touching four files this branch also changes. Its schema addition, grokNativeWorkflows, sits well below the mcpServers block edited here, and its two new endpoint tests land in classes above the ones this branch added, so both sides applied cleanly. Checked rather than assumed, since a clean auto-merge is not the same as a correct one: - The mcpServers command-or-url anyOf and its url property both survive, and the incoming grokNativeWorkflows property is present alongside them. - The schema/model parity test from #575 still passes, which it would not if only one side of #596's field had landed. - #596's own profile validates clean through the expansion ceiling added here, and its assertion on the exact shape of the schema endpoint response still holds. - The ratios documented on that ceiling are unchanged. They are stated against the largest bundled profile, so a new provider shipping a profile would have invalidated them; #596 ships none, and developer.md is still the largest at 23 expanded values and depth 3. - docs/agent-profile.md took both descriptions without duplication. The local uv.lock drift that every commit on this branch has excluded had to be stashed to take the merge, because #596 adds psutil and types-psutil to that file. Those additions survive in the working tree; the drift itself is regenerated by every `uv run`, which is why it keeps reappearing, and it still does not belong in this branch. Verified on the merged tree: 347 passed across the profile, atomic, scope and read-gating modules, and 6,939 passed on the full suite. black and isort clean across 554 files. 98 net new tests, re-measured per file against this base.
5 participants
Closes #578.
Adds the official xAI Grok Build CLI as a first-class CAO provider.
What changed
grok_cliprovider registration across the CLI, API, provider manager, workspace access, and web provider selector.GROK_HOME, auth symlinking, atomic MCP TOML configuration, CAO terminal identity, profile/model/rules support, and cleanup.--denyrules and--disable-web-searchwhenweb_fetchis unavailable.Orchestration verification
The Grok tmux E2E matrix passed 14/14 locally. It covers:
This exercises the
examples/assigntopology requested in #578: analysts complete their delegated work and communicate results back, while the supervisor coordinates and produces the report rather than doing the analyst work itself.Additional verification
git diff --check: passedScope
This PR targets the official xAI Grok CLI only, as agreed in #578. It does not add a provider-specific CI workflow or include the separate experimental Herdr work.